Skip to content

DEPR: deprecate get_ftype_counts (GH18243) #20404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

GGordonGordon
Copy link
Contributor

Deprecate NDFrame.get_ftype_counts()

@codecov
Copy link

codecov bot commented Mar 19, 2018

Codecov Report

Merging #20404 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #20404      +/-   ##
==========================================
+ Coverage   91.79%   91.85%   +0.05%     
==========================================
  Files         152      152              
  Lines       49215    49236      +21     
==========================================
+ Hits        45179    45225      +46     
+ Misses       4036     4011      -25
Flag Coverage Δ
#multiple 90.24% <ø> (+0.05%) ⬆️
#single 41.88% <ø> (+0.03%) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 95.86% <ø> (ø) ⬆️
pandas/core/arrays/categorical.py 96.2% <0%> (-0.02%) ⬇️
pandas/core/dtypes/missing.py 91.07% <0%> (ø) ⬆️
pandas/core/reshape/reshape.py 100% <0%> (ø) ⬆️
pandas/core/strings.py 98.32% <0%> (ø) ⬆️
pandas/core/indexes/datetimelike.py 96.72% <0%> (ø) ⬆️
pandas/core/base.py 96.78% <0%> (ø) ⬆️
pandas/core/panel.py 97.29% <0%> (ø) ⬆️
pandas/core/series.py 93.84% <0%> (ø) ⬆️
pandas/core/indexes/category.py 97.3% <0%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17c1fad...26268d9. Read the comment docs.

@jreback jreback added the Deprecate Functionality to remove in pandas label Mar 19, 2018
@@ -4707,6 +4709,45 @@ def get_ftype_counts(self):
object:dense 1
dtype: int64
"""
warnings.warn("get_ftype_counts is deprecated and will "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to create another function here, this is purely user facing (IOW don't create a _get_ftype_counts). keep the doc-string though.

@@ -64,7 +64,7 @@ def test_dtype(self):
assert self.ts.ftypes == 'float64:dense'
tm.assert_series_equal(self.ts.get_dtype_counts(),
Series(1, ['float64']))
tm.assert_series_equal(self.ts.get_ftype_counts(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just assert the warning here

@jreback jreback added this to the 0.23.0 milestone Mar 19, 2018
@jreback jreback added the Dtype Conversions Unexpected or buggy dtype conversions label Mar 19, 2018
@jreback jreback mentioned this pull request Mar 19, 2018
34 tasks
@pep8speaks
Copy link

pep8speaks commented Mar 20, 2018

Hello @GGordonGordon! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on March 27, 2018 at 02:35 Hours UTC

@@ -3396,8 +3396,11 @@ def map(self, mapper, na_action=None):

def isin(self, values, level=None):
"""
Return a boolean array where the index values are in `values`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know where these changes are coming from? Ideally this PR will just do the deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomAugspurger - Appears they snuck in with that large pd file under commit 4a43815 (GH20249)

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a large file called pd in your PR that'll need to be deleted (our fault).

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Mar 21, 2018 via email

noemielteto and others added 5 commits March 20, 2018 22:42
@GGordonGordon GGordonGordon force-pushed the GH18243_deprecate_get_ftype_counts branch from 4cc2d8c to 59c4bff Compare March 21, 2018 02:44
@@ -3396,11 +3396,8 @@ def map(self, mapper, na_action=None):

def isin(self, values, level=None):
"""
Return a boolean array where the index values are in `values`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert the changes here? Let me know if you need help with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomAugspurger - I had removed it as you had requested to only have the deprecation code in the branch / PR. So just to clarify you do want this code back in this branch but you don't want that large pr file back in the branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry, that's exactly right. We want everything except the large pd file.

May be easiest to just re-add them manually in a new commit, rather than trying to mess with git.

Revert the revert for just the bases.py file
@GGordonGordon
Copy link
Contributor Author

@TomAugspurger - I've added the bases.py changes back in but left the large pd file out per your request.

@TomAugspurger TomAugspurger merged commit 687cbe2 into pandas-dev:master Mar 27, 2018
@TomAugspurger
Copy link
Contributor

Thanks @GGordonGordon !

@jorisvandenbossche
Copy link
Member

@TomAugspurger the problem with the big .pd file, was that due to our rewriting of the history? I mean, will each PR that gets updated on master have this problem? (I don't see how you would get it from merging master or rebasing on master)

@TomAugspurger
Copy link
Contributor

I think it was because @GGordonGordon forked this branch from a master that included the big pd file. So anyone who created a branch during that ~7 hours might have the file in their PRs.

@jorisvandenbossche
Copy link
Member

OK, that should be not too many PRs then :)

javadnoorb pushed a commit to javadnoorb/pandas that referenced this pull request Mar 29, 2018
dworvos pushed a commit to dworvos/pandas that referenced this pull request Apr 2, 2018
kornilova203 pushed a commit to kornilova203/pandas that referenced this pull request Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Dtype Conversions Unexpected or buggy dtype conversions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEPR: deprecate get_ftype_counts
6 participants